r? @jackh726

rustbot has assigned @jackh726.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@dianne does this need crater?

@bors2 try

☀️ Try build successful (CI)
Build commit: a498031 (a4980311fb7bb9e7893708e6bd3fbbfb2819fd3d, parent: 350d0ef0ec0493e6d21cfb265cb8211a0e74d766)

@craterbot check

👌 Experiment pr-145342 created and queued.
🤖 Automatically detected try build a498031
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

How does #145374 relate to this? The code there doesn't use super let anywhere.

Format arguments, e.g. as used by write!, are also implemented in terms of super let. This is visible when printing the HIR of that test case:

f.write_fmt({
        super let args =
            (&pointee.behind_pointer(),
                &ItemIdentifier::path(&ItemIdentifier::nserror()));
        super let args =
            [format_argument::new_display(args.0),
                    format_argument::new_display(args.1)];
        format_arguments::new_v1(&["", ", "], &args)
    })

The interaction between that, old editions not treating block tail expressions as temporary drop scopes, my guard scoping PR, and the existing super let scoping bug is what caused that particular regression.

The crater run is clean. No relevant regressions or fixes.

Summary of everything so far

Edit: added regression E.

I am naming each of the regressions: A, B, C, D, E, so it's easier to refer to them in the future. For each of them, I give a link to a reproducer of the bug, and I describe when the regression occurred, the prerequisites for code to be affected by the bug, the likelihood (in my opinion) of real user code being affected, and the impact of the bug on such code.

• Regression A: pin!() has incorrect/unexpected drop order inside if-let. #145328 (comment)
  • When: Regressed from 1.87.0 to 1.88.0, presumably in Implement pin!() using super let #139114
  • Prerequisites: Use pin!(HasDrop) in an if let condition, and use the entire if let expression as a subexpression of something else.
  • Likelihood: Unlikely to affect real code, since it requires a rather specific and strange set of conditions.
  • Impact: Silent miscompile. I think it probably can also cause a compile error.
• Regression B: fix drop scope for super let bindings within if let #145342 (comment)
  • When: Regressed from 1.88.0 to 1.89.0, presumably in Allow storing format_args!() in a variable #140748
  • Prerequisites: In an if let condition, use a write!()-like macro to format two or more values, one of which has a destructor and references a temporary.
  • Likelihood: I'm not sure. It requires some specific conditions, but those conditions are not so rare.
  • Impact: Compile error. I do not know whether this can cause a silent miscompile or not.
• Regression C (Not fixed by this PR): Regression: format_args!() no longer lifetime-extends some temporaries. #145422
  • When: Regressed from 1.88.0 to 1.89.0, presumably in Allow storing format_args!() in a variable #140748
  • Prerequisites: Use format_args!() to format a value that has a destructor. Then, store the result into a variable. (The conditions are more specific than this, but I'm not sure what exactly they are.)
  • Likelihood: Very unlikely to affect real code, since there's no reason to store a format_args!() into a variable in versions 1.88.0 or earlier. Such variables became potentially usable only since version 1.89.0.
  • Impact: Compile error. I do not know whether this can cause a silent miscompile or not.
• Regression D: Regression when dropping temporary value in match in older editions #145374
  • When: Regressed from 1.89.0 to nightly, in add a scope for if let guard temporaries and bindings #143376
  • Prerequisites: Inside a match arm that has an if guard, use a write!-like macro to format two or more values, one of which has a destructor and references a temporary. This macro call is the last expression in a block. Only affects edition 2021 or earlier. (Edit: see fix drop scope for super let bindings within if let #145342 (comment) for corrections)
  • Likelihood: Already affected real code.
  • Impact: Compile error. I do not know whether this can cause a silent miscompile or not.
• Regression E (Not fixed by this PR): pin!() changed temporary lifetime extension behavior in version 1.88.0 with edition 2024 tail expression temporary scopes #145784
  • When: Regressed from 1.87.0 to 1.88.0, presumably in Implement pin!() using super let #139114
  • Prerequisites: Edition 2024 is used. Inside pin!(), there's a block expression. That block's tail expression creates a temporary with a destructor in a position that could be lifetime-extended if the pin!() were to be assigned to a let binding. The pin!() is used as a subexpression of something else in a way that can't be lifetime-extended.
  • Likelihood: Probably unlikely to affect real code, since affected code would be relying on very subtle drop order details, while also pinning a reference/pointer for some reason.
  • Impact: Silent miscompile (requires a raw pointer cast/coercion), or code compiles when it shouldn't.

Additionally, there's:

• Non-regression bug: pin!() has incorrect/unexpected drop order inside if-let. #145328 (comment)
  • When: Affects code since version 1.88.0, due to let-chains stabilizing in Stabilize let chains in the 2024 edition #132833
  • Prerequisites: In a let-chain, use pin!(HasDrop) in the second link or later of the chain, and have a temporary with a destructor earlier in the chain.
  • Likelihood: I'm not sure. It requires some specific conditions, but those conditions are not so rare.
  • Impact: Silent miscompile. I think it probably can also cause a compile error.

With regard to regression D, I imagine it affects edition 2024 as well for match arms where the arm body isn't wrapped in a block expression. It's caused by #143376 extending the surface area of the bug underlying regressions A/B to all match arms with guards where the super let-based macro isn't wrapped in a more deeply nested temporary destruction scope (e.g. a block tail expression in edition 2024). I'm sure you can also get a silent miscompile out of it if you're relying on things like temporary drop order for tuple constructor expressions and constructing a tuple with a write! call in it, but that feels unlikely in real code.

🔔 This is now entering its final comment period, as per the review above. 🔔

We discussed this in the @rust-lang/libs-api meeting and we're happy to defer to lang for this issue.

Stable backport declined as per compiler team on Zulip.

We reviewed again the stable backport and our concerns still stand.

@rustbot label -stable-nominated

@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-145342/retry-regressed-list.txt

👌 Experiment pr-145342-1 created and queued.
🤖 Automatically detected try build a498031
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Something to consider for the FCP: this PR does not make the identity in #145342 (comment) hold. See #145784 (comment) for how it fails to hold in non-extending expressions, due to super let extending the lifetimes of its initializer's temporaries, effectively ignoring block tail expressions' temporary drop scopes. If we want that identity, I think we'd need to rework non-extended super let to have more restrictive temporary lifetime extension rules. Alternatively, whatever identity is used for reasoning about super let may need to account for it having differing meanings in different contexts.

I interpret that issue as separate from the bug this PR fixes: this PR fixes super let's bindings' lifetimes, whereas the identity fails to hold in non-extending expressions due to super let's initializer's temporaries' lifetimes. However, if the FCP is for super let behaving a particular way, then this PR alone doesn't achieve that.

Edit: see #145838 for an implementation of restricting non-extended super let's temporary lifetime extension. I haven't done extensive testing yet, but it might be what's needed.

🚧 Experiment pr-145342-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

🎉 Experiment pr-145342-1 is completed!
📊 0 regressed and 0 fixed (178704 total)
📊 39455 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more